Pass DeployTarget in SDK #5616

ffjlabo · 2025-03-03T04:49:16Z

What this PR does:

as title

Why we need it:

We need DeployTarget for the Deployment plugin to use platform-specific config.

Which issue(s) this PR fixes:

Part of #5530

Does this PR introduce a user-facing change?:

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

Signed-off-by: Yoshiki Fujikane <[email protected]>

codecov · 2025-03-03T04:55:58Z

Codecov Report

Attention: Patch coverage is 14.28571% with 24 lines in your changes missing coverage. Please review.

Project coverage is 26.58%. Comparing base (4afa3e6) to head (1efc476).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/plugin/sdk/deployment.go	0.00%	19 Missing ⚠️
...p/pipedv1/plugin/kubernetes/deployment/rollback.go	0.00%	3 Missing ⚠️
...g/app/pipedv1/plugin/kubernetes/deployment/sync.go	33.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5616   +/-   ##
=======================================
  Coverage   26.58%   26.58%           
=======================================
  Files         474      474           
  Lines       50546    50564   +18     
=======================================
+ Hits        13438    13444    +6     
- Misses      36045    36057   +12     
  Partials     1063     1063

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Warashi

Thank you.
I think the spec you're trying to implement is good, and I've commented on how you've implemented it.

Warashi · 2025-03-03T05:05:50Z

pkg/plugin/sdk/deployment.go

+	dtNames, err := request.GetInput().GetDeployment().GetDeployTargets(s.commonFields.config.Name)
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "failed to get deploy targets for plugin %s: %v", s.commonFields.config.Name, err)
+	}


The plugin which doesn't have deploy target configurations such as WAIT will return an error in this if statement, and it's not expected behavior.

ref;

pipecd/pkg/model/deployment.go

Lines 217 to 224 in 4afa3e6

func (d *Deployment) GetDeployTargets(pluginName string) ([]string, error) {

dps, ok := d.GetDeployTargetsByPlugin()[pluginName]

if !ok || len(dps.GetDeployTargets()) == 0 {

return nil, fmt.Errorf("deploy target not found for plugin %v", pluginName)

}

return dps.GetDeployTargets(), nil

}

@Warashi
Thanks.
I fixed the method logic to allow the case of no deploy target. We can decide whether it is an error or not in the plugin logic from now.
0e167c0

Warashi · 2025-03-03T05:10:25Z

pkg/plugin/sdk/deployment.go

+		if dt := s.commonFields.config.FindDeployTarget(name); dt != nil {
+			var sdkDt DeployTargetConfig
+			if err := json.Unmarshal(dt.Config, &sdkDt); err != nil {
+				return nil, status.Errorf(codes.Internal, "failed to unmarshal deploy target config: %v", err)
+			}
+
+			deployTargets = append(deployTargets, &DeployTarget[DeployTargetConfig]{
+				Name:   name,
+				Labels: dt.Labels,
+				Config: sdkDt,
+			})
+		}


[IMO]
We can use the early-return pattern with the error containing the deployTarget name that is not found when dt == nil .

Fixed on 8173d91

t-kikuc

Thank you, I left some nits.

t-kikuc · 2025-03-03T05:06:45Z

pkg/plugin/sdk/deployment.go

+	}
+
+	if len(dtNames) != len(deployTargets) {
+		return nil, status.Errorf(codes.Internal, "the number of deploy targets in the piped plugin config should be the same as the ones set on the deployment")


[nits]
Let's add len(dtNames) and len(deployTargets) in the message to make it easier to troubleshoot.

Fixed on fc90b4c

t-kikuc · 2025-03-03T05:23:58Z

pkg/plugin/sdk/deployment.go

+
+	dtNames, err := request.GetInput().GetDeployment().GetDeployTargets(s.commonFields.config.Name)
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "failed to get deploy targets for plugin %s: %v", s.commonFields.config.Name, err)


[nits]
[nit]
I wanna clarify which one we mention,

the deployTraget was not found in a piped config

the deployTarget was not set in the request(deployment)

Suggested change

return nil, status.Errorf(codes.Internal, "failed to get deploy targets for plugin %s: %v", s.commonFields.config.Name, err)

return nil, status.Errorf(codes.Internal, "failed to get deploy targets from the deployment for plugin %s: %v", s.commonFields.config.Name, err)

Fixed on fbb133e

t-kikuc · 2025-03-03T05:26:18Z

pkg/plugin/sdk/deployment.go

+			var sdkDt DeployTargetConfig
+			if err := json.Unmarshal(dt.Config, &sdkDt); err != nil {
+				return nil, status.Errorf(codes.Internal, "failed to unmarshal deploy target config: %v", err)
+			}


[IMO] If possible, it's better to cache DeployTargetConfigs in the server instead of repeating unmarshaling.

@t-kikuc
Thanks, that sounds nice.
I added a TODO comment for it. I think it would be nice to focus on passing the deploy targets in this PR. Is it OK?
03519ad

Thank you! TODO is best for this case!

We don't have to do that now.

Signed-off-by: Yoshiki Fujikane <[email protected]>

…which don't need them Signed-off-by: Yoshiki Fujikane <[email protected]>

Warashi · 2025-03-03T06:32:52Z

pkg/plugin/sdk/deployment.go

+	for _, name := range dtNames {
+		dt := s.commonFields.config.FindDeployTarget(name)
+		if dt == nil {
+			continue


[IMO]
How about returning an error here? If we return here, we can remove the if len(dtNames) != len(deployTargets) block after the for loop.

@Warashi
Ah, finally I got your point.
Fixed on 1efc476

Signed-off-by: Yoshiki Fujikane <[email protected]>

Warashi

LGTM

Pass DeployTarget in SDK

d5b9bdf

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo requested review from khanhtc1202, t-kikuc and Warashi as code owners March 3, 2025 04:49

github-actions bot added the area/go label Mar 3, 2025

Remove comment

fa64fcd

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo force-pushed the extract-deploy-targets-in-sdk branch from 0b6ccd7 to fa64fcd Compare March 3, 2025 04:50

Warashi reviewed Mar 3, 2025

View reviewed changes

t-kikuc reviewed Mar 3, 2025

View reviewed changes

ffjlabo added 5 commits March 3, 2025 14:33

Refactor to do early continue

8173d91

Signed-off-by: Yoshiki Fujikane <[email protected]>

Fix error message

fbb133e

Signed-off-by: Yoshiki Fujikane <[email protected]>

Add the number of the deployTargets

fc90b4c

Signed-off-by: Yoshiki Fujikane <[email protected]>

Add comment for caching

03519ad

Signed-off-by: Yoshiki Fujikane <[email protected]>

Allow no deploy targets in the plugin config considering the plugins …

0e167c0

…which don't need them Signed-off-by: Yoshiki Fujikane <[email protected]>

github-actions bot added the area/pipedv1 label Mar 3, 2025

ffjlabo requested review from Warashi and t-kikuc March 3, 2025 06:30

Warashi reviewed Mar 3, 2025

View reviewed changes

Refactor to return error early

1efc476

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo requested a review from Warashi March 3, 2025 06:50

Warashi approved these changes Mar 3, 2025

View reviewed changes

ffjlabo enabled auto-merge (squash) March 3, 2025 06:56

t-kikuc approved these changes Mar 3, 2025

View reviewed changes

ffjlabo merged commit ce438ce into master Mar 3, 2025
17 of 18 checks passed

ffjlabo deleted the extract-deploy-targets-in-sdk branch March 3, 2025 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass DeployTarget in SDK #5616

Pass DeployTarget in SDK #5616

ffjlabo commented Mar 3, 2025

codecov bot commented Mar 3, 2025 •

edited

Loading

Warashi left a comment

Warashi Mar 3, 2025

ffjlabo Mar 3, 2025

Warashi Mar 3, 2025

ffjlabo Mar 3, 2025

t-kikuc left a comment

t-kikuc Mar 3, 2025

ffjlabo Mar 3, 2025

t-kikuc Mar 3, 2025

ffjlabo Mar 3, 2025

t-kikuc Mar 3, 2025

ffjlabo Mar 3, 2025

t-kikuc Mar 3, 2025

Warashi Mar 3, 2025

ffjlabo Mar 3, 2025

Warashi left a comment

	func (d *Deployment) GetDeployTargets(pluginName string) ([]string, error) {
	dps, ok := d.GetDeployTargetsByPlugin()[pluginName]
	if !ok \|\| len(dps.GetDeployTargets()) == 0 {
	return nil, fmt.Errorf("deploy target not found for plugin %v", pluginName)
	}

	return dps.GetDeployTargets(), nil
	}

	return nil, status.Errorf(codes.Internal, "failed to get deploy targets for plugin %s: %v", s.commonFields.config.Name, err)
	return nil, status.Errorf(codes.Internal, "failed to get deploy targets from the deployment for plugin %s: %v", s.commonFields.config.Name, err)

Pass DeployTarget in SDK #5616

Pass DeployTarget in SDK #5616

Conversation

ffjlabo commented Mar 3, 2025

codecov bot commented Mar 3, 2025 • edited Loading

Codecov Report

Warashi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-kikuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Warashi left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 3, 2025 •

edited

Loading